-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Benchmarking Scripts for Mlpack's LMNN, Shogun's LMNN & Matlab's LMNN #123
base: master
Are you sure you want to change the base?
Conversation
methods/shogun/lmnn.py
Outdated
raise Exception("unknown parameters") | ||
|
||
# Perform LMNN. | ||
prep = ShogunLMNN(feat, labels, k) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use self.k
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I again missed pushing the changes :)
@iglesias @manish7294 has some question about LMNN implementation in shogun :) |
@iglesias Hi! Hope you are doing good :) |
Hi @manish7294. So this is about KNN. What about the following: in mlpack use KNN with Does this make sense to you? |
@iglesias Thanks for looking into this. We thought about that but it would create problem in calculating predictions. As in case of Shogun's KNN prediciton, the point itself will always be counted as the positive nearest neighbor and will definitely give some amount of error in the measuring accuracy. |
Would it be possible to just get the k+1 nearest neighbors from Shogun itself? If that is the case, then we can do the classification ourselves (and ensure that we are always doing it the same way). If we can't get those from Shogun, maybe it is better to use a different technique for the nearest neighbor search. I don't think it makes such a huge difference either way, since we aren't timing the nearest neighbor search, we are just using it for finding the true nearest neighbors to calculate a metric. |
Thanks all for helping out. Hopefully, the new custom KNN accuracy predictor will work as expected :) |
methods/shogun/lmnn.py
Outdated
count += 1 | ||
|
||
accuracy = (count / NN.shape[1]) * 100 | ||
return [time, accuracy] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks correct to me. But I think maybe it would be useful to split this into a separate function that we could put into methods/metrics/
? It seems to me like we could calculate a few metrics using variations of this code (but that code could all be one function):
- 1-NN accuracy
- 3-NN accuracy
- distance-weighted 3-NN accuracy
- 5-NN accuracy
- distance-weighted 5-NN accuracy
And the input to the function could be the nearest neighbor indices and distances and the labels, plus the number of neighbors to use in the metric calculation and whether or not to use distance calculation. Does that make sense? What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems about right. But one thing bugging me --- You said we can calculate 3-NN, 5-NN from this code, which is possible but what significance would 5-NN have if we train our lmnn let's say for k = 3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, so LMNN is optimizing for k=3 if you run it with k=3, but it may still improve the 5-NN performance. Given that it would be so easy to add, I don't see any issue with doing that, but you are right, that may not be the most interesting metric to compare. Still it is not a problem to add too many metrics. :)
No problem then, I will add it for sure :) |
Everything looks good to me, but the build will fail until LMNN is part of the newest release of mlpack. So we can wait to merge until then, and we can be sure to release a newer version of mlpack soon-ish (before the end of the summer) with the LMNN support. For now, you can use the branch to run timing tests. |
Do you think it would be useful to include mlpack-master as another library, that would allow us to merge this earlier. |
@mlpack-jenkins test this please |
Can one of the admins verify this patch? |
Benchmarking scripts for Mlpack, Shogun & Matlab's LMNN.